probably correct but please give some links (eg spec, stackoverflow etc) in your PR description to double check;

tests need fixing

This is a breaking change

which code would break?
if you're talking about doAssertRaises(parseQuery("key=value&key2=x=1")) I wouldn't categorize it as a breaking change, it'd be something used only in some test suite, not actual code

@timotheecour As an example, there may be code out there which relies on being able to check for validity of the query string and explicitly checks for a ValueError.

Are there any downsides to keeping backwards compatibility here?

if the spec (or most libraries) allows =, then I'd much prefer doing a -d:nimLegacyParseQueryStrict and being correct by default than legacy behavior by default. The point is to avoid accumulating non-standard behavior over time.

I found some references for parsing query strings (and checked them):

• WhatWG's HTML5 urlencoded parser accepts literal = characters in value
• Go: https://play.golang.org/p/8Yug9Kql2CF no parsing error and = interpreted verbatim in value
• Python urllib.parse.parse_qs: same behaviour
• Ruby CGI::parse accepts = too

Updated code with @timotheecour requested changes. Add compilation flag to restore old behaviour suggested. I too prefer the new behaviour by default, it is unlikely to cause breaks in existing apps and the new behaviour is better I believe. it is also conforming to standards and to the general principle of network programming : be liberal in what you accept.

On the contrary, web apps written currently in Nim are probably not handling this specific case of query string parsing, and the new behavior can avoid an error that could in some cases provoke a denial of service.

Rebase error? Did you rebase against master instead of devel or something?

Yep, sorry, updated

don't get rid of all errors please

which other errors? can you show an input where you expect an error?

@dom96: I don't understand what you are suggesting... Should we (A) fix the parser to accept the = character and never raise an error, not bothering with compatibility define flag or (B) keep the current behaviour and not do anything, suggesting library users to fix their software to avoid the = character in the value field.

If we want to be compliant with HTML5 spec, we need to change the behaviour and this is breaking to library users that expects an exception in this special case.

If we want to keep raising an exception, we can't be compliant to the HTML spec.

Note: the = character we talk about is the only case when an error is raised. There is no other case where an error is raised as far as I know reading the code.

Updated the code.

I'm not convinced that the CI errors comes from my code, errors seems to come from fidget and PackageFileParsed (whatever those are) and I don't really see the problem looking at the CI logs...

Looking at the Python implementation it appears this is a case where an exception will be raised with strict parsing on:

>>> urllib.parse.parse_qs('&q', strict_parsing=True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.6/urllib/parse.py", line 676, in parse_qs
    max_num_fields=max_num_fields)
  File "/usr/lib/python3.6/urllib/parse.py", line 729, in parse_qsl
    raise ValueError("bad query field: %r" % (name_value,))
ValueError: bad query field: ''
>>> urllib.parse.parse_qs('&q', strict_parsing=False)
{}

And you get an exception with decodeQuery for this too.

Another thing to keep in mind here is that cgi makes use of this code, and explicitly checks for exceptions, see the diff that moved this iterator: 6895040. Are we sure this isn't breaking CGI?

@xflywind any thoughts?

@dom96 You're right, cgi makes use of the same code (and I wanted originally to fix the cgi.decodeData iterator. I am removing the error check in the code as this is no longer necessary and updating the changelog.

As for strict checking, I believe this is not the same thing. Is there a use case for struct checking? Nor all languages advertise such a feature. In any case I believe this would belong to another PR and should be clearly defined before any implementation is attempted.

Okay, so yes you can make these changes. This code is actually brand new and hasn't been released yet in a Nim version (it was only added 16 days ago), so you don't need to add it to the changelog, nor do you need those nimLegacyParseQueryStrict defines.

The only concern I have is with any breakage in cgi, I can see that a test was added to check that the exception is raised correctly in 6895040.

As for strict_parsing, yes we can add it separately. We should really just copy the Python implementation IMO (it includes other things like support for ; as separators) but that can come in follow up PRs.

Well, actually the code in uri right now comes from cgi, it was just moved over. Perhaps the compatibility option should be renamed to refer to cgi instead. I'm not entirely sure it is needed though.

Also, I'm not sure that there is a need for a strict parsing as this does not seems to be a well defined behavior. There is no common implementation of such a strict mode across all languages and I'm not sure copying the Python behavior is of any use on its own. It looks like unneeded complexity for me.

Also, I'm not sure that there is a need for a strict parsing as this does not seems to be a well defined behavior.

which of the languages you mentioned in #16647 (comment) define some notion of strict parsing? I'm fine without having such a "strict parsing" option, but if lots of languages have such a notion, then maybe we can reconsider (in future work)

#16647 (comment) which of the languages you mentioned in #16647 (comment) define some notion of strict parsing?

Of those languages, only Python seems to have strict parsing. Ruby does not neither does Go. Same for the extra options which are only available in Python.

Of those languages, only Python seems to have strict parsing. Ruby does not neither does Go. Same for the extra options which are only available in Python.

ok, that's convincing. No need to support a notion of strict parsing then.

Looks really good this way. Follup PRs can address potential shortcomings that I've missed.